Remove djsonb dependency and move its relevant logic into Grout core #7

jeancochrane · 2018-07-23T19:03:13Z

Overview

Now that JSONB is supported in Django core, we no longer need most of the custom djsonb package. This PR removes the external dependency on djsonb and moves its lookup logic into Grout core. It also moves the djsonb test suite into this package in order to confirm that JSON filtering logic works.

Notes

I had to make a few small changes to the djsonb lookup logic in order to get it to work with Django's built-in JSONField type. Since those changes won't show up in this diff, I opened a separate PR over in djsonb in order to expose the diff.
The Lookup module is the only part of djsonb that we need to preserve, since it provides all of the logic that parses a query in the format that the Grout API expects (like jsonb={'a': {'b': {'c': {'_rule_type': 'containment', 'contains': [1, 2]}}}}) and turns it into SQL.
- Note that by using the old lookup logic, we're not yet leveraging the main advantage of Django's JSONField type, which is the built-in query syntax. I didn't want to change the query syntax in this PR, however, since that feels like part of Improve the syntax for JSON querying/filtering grout-2018-fellowship#16.
We had chatted in standup about whether it would be a better idea to update djsonb or bring the relevant portions into this package. Since all I need to preserve of djsonb is the lookup logic, I decided it would be best to bring it in here.
Normally, editing fields in old migrations is a no-go for backwards compatibility. However, I decided to do it in this PR for two reasons:
1. If we can edit the old migrations to change djsonb.JsonBField types to the built-in JSONField type, we can purge the djsonb dependency entirely
2. Since no apps are currently using Grout, migrations don't have to be reverse-compatible

Testing

Travis will run unit tests. To run them locally:

# Update containers.
./scripts/update

# Run tests.
./scripts/test

Closes azavea/grout-2018-fellowship#12.

flibbertigibbet

Looks like the migrations could use a little cleanup. On running the test script:

CommandError: Conflicting migrations detected; multiple leaf nodes in the migration graph: (0024_auto_20180716_1605, 0023_auto_20180717_1003 in grout).```

jeancochrane · 2018-07-23T20:03:25Z

I don't see the migration labeled 0023_auto_20180717_1003 anywhere in the migrations folder in this branch. Any chance it's a holdover from another branch @flibbertigibbet?

flibbertigibbet · 2018-07-23T20:06:08Z

Sure enough, I guess that one came from the remove-location-fields branch 👍

flibbertigibbet

🎊

Just a few notes. I've run the tests locally, but haven't tested it otherwise. Looking forward to seeing this all come together!

flibbertigibbet · 2018-07-23T21:30:12Z

CHANGELOG.md

@@ -5,6 +5,8 @@
 Changes we're making en route to version 2.0. When we cut a release, this
 section will form the changelog for v2.0.

+- Removed external `djsonb` dependency and moved its lookup logic into
+  Grout core ([#7](https://github.com/azavea/grout/pull/7)


flibbertigibbet · 2018-07-23T21:33:22Z

grout/lookups.py

+
+
+class FilterTree:
+    """This class should properly assemble the pieces necessary to write the WHERE clause of


Nitpick: could this block comment be rephrased to open with a single line, separated from the rest with a blank line, per the docstring PEP?

Can do. I'll flesh out all the docstrings in this module.

flibbertigibbet · 2018-07-23T21:36:42Z

grout/lookups.py

+
+        for rule in self.rules:
+            # If not a properly registered rule type
+            if not self.is_rule(rule[1]):


How do we know index 1 will be in bounds for rule? Could it be made more obvious what data is at the 0 and 1 offsets?

Great question! Based on the parameter list for the methods that use these values (like FilterTree.containment_filter), it looks like rule[0] corresponds to the path of the nested query, while rule[1] corresponds to the rule itself. Both elements have to exist, since self.get_rules can only return either A) an empty list or B) a list of two-tuples.

I'll add this info in a comment and redo the iteration assignment to change it from:

for rule in self.rules:

To something more expressive;

for path, rule in self.rules:

flibbertigibbet · 2018-07-23T21:39:33Z

grout/lookups.py

+    @classmethod
+    def containment_filter(cls, path, rule):
+        """Filter for objects that contain the specified value at some location"""
+        template = reconstruct_object(path[1:])


Similar to the questions about rule above, could it be made more obvious what the slices of path contain, and why it's safe to assume path has a length of at least two?

Add support for Django 2.0

jeancochrane added 2 commits July 23, 2018 14:38

Remove djsonb dependency and move its relevant logic into grout core

fbfc9ea

Update changelog

6c781ba

jeancochrane mentioned this pull request Jul 23, 2018

Use built-in JSONField azavea/djsonb#16

Closed

jeancochrane requested a review from flibbertigibbet July 23, 2018 19:14

flibbertigibbet suggested changes Jul 23, 2018

View reviewed changes

Support Django 2.0

530a2e4

Update changelog

5f43e80

Cap Django dependency version at 2.0

ec53a69

jeancochrane mentioned this pull request Jul 23, 2018

Add support for Django 2.0 #8

Merged

flibbertigibbet approved these changes Jul 24, 2018

View reviewed changes

jeancochrane and others added 2 commits July 24, 2018 14:15

Merge pull request #8 from azavea/django-2.0

e9ea19b

Add support for Django 2.0

Clarify lookup code and flesh out docstrings

d6b7de3

jeancochrane merged commit c217cda into develop Jul 24, 2018

jeancochrane deleted the remove-djsonb branch July 24, 2018 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove djsonb dependency and move its relevant logic into Grout core #7

Remove djsonb dependency and move its relevant logic into Grout core #7

jeancochrane commented Jul 23, 2018 •

edited

Loading

flibbertigibbet left a comment

jeancochrane commented Jul 23, 2018

flibbertigibbet commented Jul 23, 2018

flibbertigibbet left a comment

flibbertigibbet Jul 23, 2018

flibbertigibbet Jul 23, 2018

jeancochrane Jul 24, 2018

flibbertigibbet Jul 23, 2018

jeancochrane Jul 24, 2018

flibbertigibbet Jul 23, 2018



		class FilterTree:
		"""This class should properly assemble the pieces necessary to write the WHERE clause of

Remove djsonb dependency and move its relevant logic into Grout core #7

Remove djsonb dependency and move its relevant logic into Grout core #7

Conversation

jeancochrane commented Jul 23, 2018 • edited Loading

Overview

Notes

Testing

flibbertigibbet left a comment

Choose a reason for hiding this comment

jeancochrane commented Jul 23, 2018

flibbertigibbet commented Jul 23, 2018

flibbertigibbet left a comment

Choose a reason for hiding this comment

flibbertigibbet Jul 23, 2018

Choose a reason for hiding this comment

flibbertigibbet Jul 23, 2018

Choose a reason for hiding this comment

jeancochrane Jul 24, 2018

Choose a reason for hiding this comment

flibbertigibbet Jul 23, 2018

Choose a reason for hiding this comment

jeancochrane Jul 24, 2018

Choose a reason for hiding this comment

flibbertigibbet Jul 23, 2018

Choose a reason for hiding this comment

jeancochrane commented Jul 23, 2018 •

edited

Loading